-
Notifications
You must be signed in to change notification settings - Fork 589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixing overflow in GatkSparkTool.getRecommendedNumReducers() #5586
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5586 +/- ##
==============================================
+ Coverage 87.04% 87.049% +0.009%
- Complexity 31490 31527 +37
==============================================
Files 1924 1928 +4
Lines 145189 145340 +151
Branches 16082 16089 +7
==============================================
+ Hits 126373 126517 +144
- Misses 12958 12963 +5
- Partials 5858 5860 +2
|
@@ -365,8 +366,8 @@ public int getRecommendedNumReducers() { | |||
if (numReducers != 0) { | |||
return numReducers; | |||
} | |||
int size = readInputs.keySet().stream().mapToInt(k -> (int) BucketUtils.dirSize(k)).sum(); | |||
return 1 + (size / getTargetPartitionSize()); | |||
long size = readInputs.keySet().stream().mapToLong(k -> BucketUtils.dirSize(k)).sum(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an explicit check for overflow before downcasting the long?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good
int size = readInputs.keySet().stream().mapToInt(k -> (int) BucketUtils.dirSize(k)).sum(); | ||
return 1 + (size / getTargetPartitionSize()); | ||
long size = readInputs.keySet().stream().mapToLong(k -> BucketUtils.dirSize(k)).sum(); | ||
return 1 + Math.toIntExact(size / getTargetPartitionSize()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are going to use a throwing method you should at least catch this exception and give a helpful error message, as some user/developer may want to increase the target partition size to compensate or some other such thing.
@droazen @jamesemery I've made changes, could someone take a look if you get a chance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now 👍 feel free to merge
responded to these comments and James did a re-review
No description provided.